-
-
Notifications
You must be signed in to change notification settings - Fork 19
Improve DavException
construction
#83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves DavException
construction by refactoring how HTTP responses are handled in exceptions and simplifying the exception hierarchy. The changes focus on making exception construction more type-safe and removing serialization concerns.
- Restructured
DavException
to have a primary constructor with simple parameters and a secondary constructor for HTTP response parsing - Added validation to specific HTTP exception subclasses to ensure they're only created with the correct status codes
- Simplified exception construction logic and removed serialization support
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
src/main/kotlin/at/bitfire/dav4jvm/exception/DavException.kt | Major refactoring of primary constructor and HTTP response parsing logic |
src/main/kotlin/at/bitfire/dav4jvm/exception/HttpException.kt | Simplified to only accept Response objects, removed status code constructor |
src/main/kotlin/at/bitfire/dav4jvm/exception/*Exception.kt | Added status code validation to specific HTTP exception subclasses |
src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt | Updated exception construction and simplified checkStatus method |
src/main/kotlin/at/bitfire/dav4jvm/Error.kt | Made Error a data class for better toString and comparison |
src/test/kotlin/at/bitfire/dav4jvm/exception/*Test.kt | Updated tests to reflect new property names and construction patterns |
gradle/libs.versions.toml | Added SpotBugs annotations dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/main/kotlin/at/bitfire/dav4jvm/exception/UnauthorizedException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/at/bitfire/dav4jvm/exception/ServiceUnavailableException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/at/bitfire/dav4jvm/exception/PreconditionFailedException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/at/bitfire/dav4jvm/exception/NotFoundException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/at/bitfire/dav4jvm/exception/ForbiddenException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/at/bitfire/dav4jvm/exception/ConflictException.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the DavException
construction to improve its design and usability. The main purpose is to decouple HTTP response parsing from the primary constructor while maintaining backwards compatibility and improving error handling.
Key Changes
- Changed
DavException
primary constructor to accept simple values instead of HTTP response objects - Added validation to HTTP exception subclasses to ensure correct status codes
- Made
Error
class a data class for better debugging and comparison capabilities
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/main/kotlin/at/bitfire/dav4jvm/exception/DavException.kt | Complete refactor of exception construction with new primary constructor and response parsing logic |
src/main/kotlin/at/bitfire/dav4jvm/exception/HttpException.kt | Simplified to only accept Response objects, removing dual constructor pattern |
src/main/kotlin/at/bitfire/dav4jvm/exception/*.kt | Added status code validation to specific HTTP exception classes |
src/main/kotlin/at/bitfire/dav4jvm/Error.kt | Converted to data class for better toString() and equality support |
src/test/kotlin/at/bitfire/dav4jvm/exception/DavExceptionTest.kt | Comprehensive rewrite of tests to cover new functionality |
src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt | Updated to use new exception constructors and simplified checkStatus method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
After doing some research and thinking about
I came to this proposal that shall close #80:
Same for the request body.
DavException
handles those cases and doesn't set a request/response excerpt then. Also added the JSR 305@WillNotClose
to make clear that theDavException
won't close the response (so it's the responsibility of the caller, who should always useuse
ortry/finally
).It's handy for constructing the response. This pattern is also used for instance in Ktor's ResponseException.
True → added test for JVM serialization, because
Exception
base class isSerializable
.Only for construction. To make this clear, I changed the primary constructor so that it takes only simple values. HttpResponse parsing is moved to a secondary constructor.
Further notes / changes:
InvalidPropertyException
a subclass ofDavException
.DavResource.checkStatus()
.Error
a data class so that it prints the error names intoString()
and errors can be compared.